-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add typechecking to default parcelconfig #4025
Conversation
Benchmark Resultspackages/benchmarks/kitchen-sink ✅
Timings
Cold Bundles
Cached Bundles
packages/benchmarks/react-hn ✅
Timings
Cold BundlesNo bundle changes detected. Cached Bundles
packages/benchmarks/ak-editor ✅
Timings
Cold Bundles
Cached Bundles
|
Gonna close this... |
@DeMoorJasper it's not clear from looking at this why it was closed? |
@uglycoyote it had gotten too much merge conflicts and I doubt it would've ever gotten merged. From the issue it seems there's almost just as much people who want this as don't want this |
@DeMoorJasper - I think you're referring to #4022, right? My reading of that (very long, very contentious) thread was that there were at least two issues being discussed:
It seemed like a lot of folks who were new to parcel didn't understand this distinction at first (until this comment helped clarify toward the end of the discussion). It seems accurate to say that issue 1 was very contentious because there are some very solid performance-related reasons to prefer babel. For issue number 2, it seemed more one-sided in favor of turning on semantic checking by default (although not 100% unanimous). I'd love to see a wider discussion around this comment, which posed the question directly. My two-cents as a heavy typescript user is that I pretty much always want to get semantic type error feedback from my build tool, so it would be great to have this be the default. This is for two main reasons:
|
@astegmaier You’re right opinions were very mixed. I don’t mind typechecking being the default The main reason for closing this was merge conflicts as I have enough PRs that need rebasing and have a higher chance of getting merged soonish |
Yeah no worries. If the discussion develops and we get everyone on board with the idea of turning this on by default, I'm happy to take care of the PR. |
Thanks for the replies. I didn't mean to cause a firestorm and I know there's a bit of a debate about this contentious issue on #4022, which I have replied on a few times. But I was curious because the effort to change the default behaviour was started by someone who's a primary contributor and then abruptly stopped without an explanation. Upon reading this, since the "gonna close this" comment was one day after posting the benchmarks, It made me wonder if the performance numbers in the benchmarks are what clinched the decision to close it, but it sound like it was other issues. I looked at the benchmarks but there were too many numbers and I didn't know what the format of the report was, I could not really tell whether they said something interesting about the performance implications of this change. I'm aware that performance is the big argument in favour of leaving it the way it was (with no use of typechecking) but had not seen any figures about what the impact of enabling typechecking was by default. |
Correct me if I'm wrong, but I don't think turning on parcel/packages/core/core/src/Parcel.js Lines 268 to 284 in 7c0aa3d
(It's a different story for using @parcel/transformer-typescript-tsc` over babel during the transform phase - see this comment). |
Please continue this discussion on #4022 rather than on this closed PR. |
↪️ Pull Request
This PR enabled TS typechecking by default. This is probably more expected behaviour than the current silent behaviour.
Related #4022
✔️ PR Todo